Walkthrough

Introduces an internal onPageReady(callback) wrapper that centralizes readiness handling via document.readyState and the "readystatechange" event; replaces prior initialization path to drive setupPageNavigationListeners. Tests updated to reflect readystatechange semantics. System specs removing pro/immediate-hydration contexts and two dependent tests.

Changes

Sequence Diagram(s)

sequenceDiagram
    participant Caller as onPageLoaded/onPageUnloaded
    participant OnReady as onPageReady(callback)
    participant Document as document
    participant Setup as setupPageNavigationListeners

    Caller->>OnReady: request setup
    OnReady->>Document: read `readyState`
    alt readyState == "complete"
        OnReady->>Setup: invoke callback immediately
    else readyState != "complete"
        OnReady->>Document: addEventListener("readystatechange", handler)
        Document->>OnReady: "readystatechange" fires
        OnReady->>OnReady: re-check `readyState`
        OnReady->>Setup: invoke callback
        OnReady->>Document: removeEventListener("readystatechange", handler)
    end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• Pay attention to:
  • Correct add/remove of the readystatechange listener and that the handler re-enters the wrapper.
  • Tests properly simulating document.readyState transitions and firing readystatechange.
  • Deleted system specs—verify CI expectations and no dependent tests remain.

Possibly related PRs

• Improve naming clarity in pageLifecycle.ts #1762 — Modifies the same page lifecycle initialization flow and naming around setup/initialize functions.
• Fix integration tests for immediate_hydration Pro license changes #1783 — Touches pro/immediate-hydration test behavior and stubbing pro utilities; overlaps with removed system specs.

Suggested reviewers

• AbanoubGhadban
• alexeyr-ci2

Poem

🐰
I wait until the document says it's "complete" and then I hop,
If not, I listen for readystatechange and stop,
I bind once, call once, then tidy up and flee,
A tiny rabbit watcher, ready when ready — whee! 🥕

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Code Review Feedback

Summary

This PR addresses a critical race condition where components may attempt to hydrate before all subresources (including component registrations) are loaded. The change moves from waiting for DOMContentLoaded to waiting for document.readyState === 'complete', which ensures all subresources are loaded before hydration begins.

✅ Positives

1. Fixes Critical Race Condition: The change correctly addresses the issue where component registration may not be complete when hydration starts, which could cause CI failures.

2. Correct Event Strategy: Using readystatechange with document.readyState === 'complete' is the right approach for waiting for all subresources (scripts, stylesheets, images) to load, not just DOM parsing.

3. Cleaner Architecture: The refactor simplifies the code by:

   • Removing the global isPageLifecycleInitialized flag
   • Creating a more focused onPageReady function
   • Eliminating the initializePageEventListeners wrapper

⚠️ Concerns & Issues

1. Potential Memory Leak (Critical)

The current implementation has a memory leak in the recursive pattern:

function onPageReady(callback: () => void) {
  if (document.readyState === 'complete') {
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      onPageReady(callback);  // ⚠️ Recursive call before removeEventListener
      document.removeEventListener('readystatechange', onReadyStateChange);
    });
  }
}

Problem: If readyState transitions from 'loading' → 'interactive' → 'complete', the listener will fire twice:

• First fire: readyState === 'interactive', recursively adds another listener
• Second fire: readyState === 'complete', executes callback
• The first listener's removeEventListener runs AFTER the recursive call, leaving potential cleanup issues

Recommended Fix:

function onPageReady(callback: () => void) {
  if (document.readyState === 'complete') {
    callback();
  } else {
    const onReadyStateChange = () => {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', onReadyStateChange);
        callback();
      }
    };
    document.addEventListener('readystatechange', onReadyStateChange);
  }
}

2. Multiple Calls to setupPageNavigationListeners

Since onPageReady is called from both onPageLoaded and onPageUnloaded, and these functions can be called multiple times, setupPageNavigationListeners will be invoked multiple times without protection.

Problem: This could lead to:

• Multiple event listeners being registered for the same events
• runPageLoadedCallbacks() being called multiple times immediately

Recommended Fix: Restore the initialization guard:

let isPageLifecycleInitialized = false;

function onPageReady(callback: () => void) {
  if (isPageLifecycleInitialized) {
    return;
  }
  
  if (document.readyState === 'complete') {
    isPageLifecycleInitialized = true;
    callback();
  } else {
    const onReadyStateChange = () => {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', onReadyStateChange);
        isPageLifecycleInitialized = true;
        callback();
      }
    };
    document.addEventListener('readystatechange', onReadyStateChange);
  }
}

3. Missing Window Check

The original code had if (typeof window === 'undefined') return; to handle SSR scenarios. This check is now missing.

Impact: While the calling code in clientStartup.ts checks for document, it's better defensive programming to keep the check in onPageReady as well, especially since it's exported and could be called from other contexts.

Recommended Fix: Add the check at the beginning of onPageReady.

4. Performance Impact (Minor)

Waiting for document.readyState === 'complete' means waiting for ALL resources (images, stylesheets, etc.) to load, which could delay hydration compared to DOMContentLoaded.

Question: Have you measured the performance impact? For pages with large images or slow-loading resources, this could noticeably delay interactivity.

Alternative Consideration: Since the core issue is component registration race conditions, have you considered:

• Ensuring component bundles are loaded with proper defer or module script ordering?
• Using a more targeted approach that waits only for script resources, not all resources?

📝 Additional Suggestions

1. Add Tests: This critical lifecycle code has no test coverage. Consider adding unit tests for:

   • Various readyState transitions
   • Multiple calls to onPageLoaded/onPageUnloaded
   • Navigation library detection timing

2. Add JSDoc Comments: Document the behavior change:

   /**
    * Waits for all page resources to load before executing callback.
    * Uses document.readyState === 'complete' to ensure all subresources
    * (scripts, stylesheets, images) are loaded, preventing race conditions
    * with component registration.
    */

3. Consider Feature Detection: If the performance impact is too high, consider a hybrid approach that only waits for complete when async component loading is detected.

🎯 Verdict

Concept: ✅ Correct approach to fixing the race condition

Implementation: ⚠️ Needs fixes for memory leak and duplicate execution issues

Priority: Fix the critical issues (#1 and #2) before merging, as they could cause production bugs that are harder to debug than the original race condition.

Review based on repo's CLAUDE.md guidelines. TypeScript/React on Rails best practices applied.

Code Review: Only initiate hydration after all subresources loaded

Thank you for this PR @Judahmeek! I've reviewed the changes and here's my feedback:

Summary

This PR changes the page lifecycle initialization to wait for document.readyState === 'complete' instead of DOMContentLoaded, ensuring all subresources (images, stylesheets, scripts) are loaded before hydration begins. This prevents component registration race conditions.

✅ Strengths

1. Addresses Real Issue: The PR fixes a legitimate race condition where components may try to hydrate before their registration code has loaded
2. Clean Implementation: The onPageReady function is well-structured with proper cleanup of event listeners
3. Test Coverage: Tests have been updated to match the new behavior
4. Backward Compatibility: Maintains the same public API

🔍 Issues & Concerns

1. Critical Logic Bug in onPageReady

The recursive call pattern creates an infinite loop risk:

function onPageReady(callback: () => void) {
  if (isPageLifecycleInitialized) {
    return;  // ❌ Returns without calling callback!
  }
  isPageLifecycleInitialized = true;
  
  if (document.readyState === 'complete') {
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      onPageReady(callback);  // ❌ Calls itself recursively
      document.removeEventListener('readystatechange', onReadyStateChange);
    });
  }
}

Problems:

• Line 74: onPageReady(callback) calls itself recursively, but isPageLifecycleInitialized is already true
• This means the callback will never execute on subsequent calls
• The guard at line 65-67 prevents the callback from running

Expected behavior: The event listener should check document.readyState === 'complete' directly and call the callback, not recursively call onPageReady.

Suggested fix:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (isPageLifecycleInitialized) {
    return;
  }
  isPageLifecycleInitialized = true;

  if (document.readyState === 'complete') {
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      if (document.readyState === 'complete') {
        callback();
        document.removeEventListener('readystatechange', onReadyStateChange);
      }
    });
  }
}

2. Test Naming Inconsistency

Line 84: Test name says "interactive" but sets readyState to "loading":

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('loading');  // ❌ Mismatch!

This should either test the "interactive" state or rename to match "loading".

3. Missing Test Coverage

The tests don't verify that the callback actually executes when readystatechange fires. Current tests only verify that:

• The event listener is added
• The callback doesn't run immediately

But they don't test:

• Does the callback run when readyState becomes 'complete'?
• Does the event listener properly clean up?

4. Performance Consideration

Waiting for readyState === 'complete' delays hydration until all resources load (images, fonts, etc.), which could be significantly slower than DOMContentLoaded.

Questions:

• Have you measured the performance impact on real applications?
• Could we use a more targeted approach (e.g., wait for specific script bundles)?
• What about progressive hydration for critical components?

5. Broader Architecture Question

The PR description mentions this is needed because async hydration moved to Pro. Some questions:

• Is delaying all hydration the right solution, or should we focus on making component registration more robust?
• Could we use a registry promise/ready state instead of relying on page load events?
• Would a manifest of required bundles be more explicit than implicit timing?

🧪 Testing Recommendations

1. Add integration test: Verify the actual callback execution when readyState changes
2. Add concurrent call test: Test what happens when onPageLoaded is called multiple times rapidly
3. Add slow resource test: Verify behavior when resources take a long time to load

📋 Action Items

Must fix before merge:

1. ✅ Fix the recursive call bug in onPageReady
2. ✅ Fix test name on line 84 or change the readyState to match
3. ✅ Add test coverage for callback execution on readystatechange

Should consider:

1. ⚠️ Measure and document performance impact
2. ⚠️ Consider alternative solutions (registry-based readiness?)
3. ⚠️ Add migration guide if this affects hydration timing significantly

🎯 Verdict

Status: Needs Changes ⚠️

The approach is sound, but the implementation has a critical bug that will prevent callbacks from executing. Once fixed and tested, this should address the race condition effectively.

Please let me know if you'd like me to clarify any of these points or help with the fixes!

Review generated using repository conventions from CLAUDE.md

Code Review for PR #1988

Summary

This PR changes the page lifecycle initialization to wait for document.readyState === 'complete' instead of \!== 'loading', ensuring hydration only begins after all subresources (images, stylesheets, scripts) are loaded. This addresses component registration race conditions that emerged after the CallbackRegistry was moved to Pro-only in PR #1841.

✅ Positive Aspects

1. Addresses Real Issue: The race condition fix is legitimate - ensuring all subresources load before hydration prevents component registration timing issues.

2. Test Coverage Updated: Tests properly updated to reflect new behavior with readystatechange events.

3. Clean Event Listener Management: Proper cleanup with removeEventListener in the onReadyStateChange handler prevents memory leaks.

4. Maintains SSR Safety: Correctly preserves typeof window === 'undefined' check for server-side rendering.

🔴 Critical Issues

1. Breaking Behavioral Change

Location: pageLifecycle.ts:70-71

// BEFORE: Runs on 'interactive' OR 'complete'
if (document.readyState \!== 'loading') {

// AFTER: Only runs on 'complete'  
if (document.readyState === 'complete') {

Problem: This significantly delays hydration. The interactive state means DOM is ready but subresources (images, fonts, CSS) may still be loading. For most React apps, starting hydration at interactive is perfectly safe and provides better UX.

Impact: Users will see a longer delay before React components become interactive, especially on slow networks where images/fonts take time to load.

Recommendation: Consider a more nuanced approach:

• Default to 'interactive' for better UX
• Add a configuration option for apps that need 'complete' due to specific timing requirements
• Or document this as a breaking change with clear rationale

2. Incorrect Event Listener Choice

Location: pageLifecycle.ts:73

document.addEventListener('readystatechange', function onReadyStateChange() {
  onPageReady(callback);
  document.removeEventListener('readystatechange', onReadyStateChange);
});

Problem: Using readystatechange with recursive calls to onPageReady() is unnecessarily complex and fragile.

Issues:

• readystatechange fires multiple times (loading → interactive → complete)
• Recursive call to onPageReady() on each state change is confusing
• The isPageLifecycleInitialized guard prevents re-initialization, but the pattern is error-prone

Better approach (if waiting for 'complete' is truly necessary):

if (document.readyState === 'complete') {
  callback();
} else {
  document.addEventListener('load', callback, { once: true });
}

Or using readystatechange with explicit state check:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    callback();
    document.removeEventListener('readystatechange', onReadyStateChange);
  }
});

3. Potential Infinite Recursion Risk

Location: pageLifecycle.ts:73-76

While the isPageLifecycleInitialized flag prevents actual infinite recursion, calling onPageReady(callback) from within the readystatechange listener is confusing and makes the control flow hard to follow.

Current flow:

1. onPageReady() adds readystatechange listener
2. State changes from loading → interactive
3. Listener calls onPageReady() again (recursive call)
4. isPageLifecycleInitialized returns early
5. State changes from interactive → complete
6. Listener calls onPageReady() again
7. Callback finally executes

This is unnecessarily convoluted.

⚠️ Moderate Issues

4. Test Name Mismatch

Location: pageLifecycle.test.js:84

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('loading'); // ← Test says 'interactive' but sets 'loading'

Issue: Test description says "interactive" but actually tests "loading" state. This is confusing and indicates the test was updated but the description wasn't properly adjusted.

Recommendation: Either:

• Update test name to: 'should wait for readystatechange when document.readyState is "loading"'
• Add a separate test for 'interactive' state behavior

5. Missing Test Coverage

The PR doesn't test the actual readystatechange event firing behavior. Current tests only verify that the listener is added, not that the callback executes when the state actually changes.

Recommended additional test:

it('should execute callback when readyState changes to complete', () => {
  setReadyState('loading');
  const callback = jest.fn();
  const { onPageLoaded } = importPageLifecycle();
  
  onPageLoaded(callback);
  expect(callback).not.toHaveBeenCalled();
  
  // Simulate state change
  setReadyState('complete');
  // Manually trigger the event since jsdom doesn't auto-fire it
  const event = new Event('readystatechange');
  document.dispatchEvent(event);
  
  expect(callback).toHaveBeenCalledTimes(1);
});

📊 Performance Considerations

Potential Regression: Waiting for 'complete' instead of 'interactive' can add hundreds of milliseconds to Time-to-Interactive (TTI), especially on:

• Slow networks
• Pages with many images
• Pages with web fonts
• Pages with large CSS files

Recommendation: Add performance monitoring or at least document this trade-off clearly.

🔒 Security Concerns

No security issues identified. The change doesn't introduce XSS, injection, or other security vulnerabilities.

💡 Alternative Solutions

Instead of this breaking change, consider:

1. Restore CallbackRegistry to core (as mentioned in PR description) - This would allow async component registration without timing changes

2. Add configuration option:

   ReactOnRails.configure({
     hydrationTiming: 'interactive' | 'complete' // Default: 'interactive'
   });

3. Component-level control: Let components opt-in to delayed hydration if needed

4. Better error handling: Add warnings when components aren't registered yet, rather than delaying all hydration

📋 Recommendations Summary

Before merging:

1. ✅ Fix recursive onPageReady() call - Use direct state check in event listener
2. ✅ Fix test name mismatch - Update test description for 'interactive' test
3. ✅ Add proper event simulation test - Verify actual state change behavior
4. ⚠️ Document breaking change - Clearly explain performance impact in CHANGELOG
5. ⚠️ Consider configuration option - Allow users to choose timing strategy
6. ⚠️ Evaluate alternatives - Is this the right fix, or should CallbackRegistry return to core?

Code Quality: 6/10 - Functional but suboptimal implementation
Test Coverage: 7/10 - Tests updated but missing event firing validation
Performance Impact: Negative - Will delay interactivity noticeably
Security: ✅ No concerns

🎯 Suggested Code Fix

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (isPageLifecycleInitialized) {
    return;
  }
  isPageLifecycleInitialized = true;

  if (document.readyState === 'complete') {
    callback();
  } else {
    // Use 'load' event which fires once when readyState becomes 'complete'
    window.addEventListener('load', callback, { once: true });
  }
}

This is clearer, more efficient, and avoids the recursive pattern.

Would you like me to elaborate on any of these points or provide additional code examples?

Code Review

Summary

This PR addresses a critical race condition in component hydration by ensuring that page lifecycle initialization only occurs after all subresources (images, stylesheets, scripts) are fully loaded. The change moves from DOMContentLoaded to readystatechange with a complete check, preventing premature component registration that can cause hydration failures.

✅ Strengths

1. Solves a Real Problem

• Addresses the component registration race condition documented in the CI failure
• Restores behavior from PR #1540 that was inadvertently lost when CallbackRegistry moved to Pro in PR #1841
• Ensures React components don't attempt to hydrate before their bundles are fully loaded

2. Clean Implementation

• The refactoring from initializePageEventListeners to onPageReady improves code clarity
• Proper cleanup with removeEventListener prevents memory leaks
• Self-documenting function name (onPageReady) better expresses intent

3. Test Coverage

• Tests updated to reflect new behavior
• Covers edge cases: complete, loading, and interactive states
• Tests verify listener cleanup and prevent duplicate initialization

⚠️ Issues & Concerns

1. Critical Bug: Infinite Recursion Risk

Location: pageLifecycle.ts:73-76

document.addEventListener('readystatechange', function onReadyStateChange() {
  onPageReady(callback);  // ⚠️ This recursively calls onPageReady
  document.removeEventListener('readystatechange', onReadyStateChange);
});

Problem: The onPageReady function checks isPageLifecycleInitialized and returns early if true. However, on the first readystatechange event that fires when state is NOT complete, this creates a loop:

1. readystatechange fires (state = loading → interactive)
2. Calls onPageReady(callback)
3. isPageLifecycleInitialized is already true, so returns early
4. Callback never executes
5. Listener is removed
6. When state reaches complete, nothing happens

Solution: Move the guard check or explicitly check for complete state:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    callback();
    document.removeEventListener('readystatechange', onReadyStateChange);
  }
});

2. Test Naming Inconsistency

Location: pageLifecycle.test.js:84

The test is named:

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('loading');  // ⚠️ But sets state to 'loading', not 'interactive'

This is misleading. Either rename the test or create a separate test for interactive state.

3. Missing Test Case: interactive State

The original code checked !== 'loading' which would trigger immediately for interactive. The new code only triggers for complete. This is likely intentional (to wait for all subresources), but there's no test verifying that interactive state correctly waits for complete.

Suggested test:

it('should wait for complete even when document.readyState is "interactive"', () => {
  setReadyState('interactive');
  const callback = jest.fn();
  const { onPageLoaded } = importPageLifecycle();

  onPageLoaded(callback);

  expect(callback).not.toHaveBeenCalled();
  expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function));
});

4. Potential Performance Impact

Waiting for complete means waiting for:

• All images
• All stylesheets
• All subframes
• All async scripts

For pages with large images or slow CDNs, this could noticeably delay hydration. Consider:

• Adding documentation about this tradeoff
• Providing a configuration option for users who want faster (but potentially racy) hydration
• Using DOMContentLoaded + a timeout as fallback

🔍 Security Concerns

None identified. The changes don't introduce XSS vectors, use proper event cleanup, and don't modify user-supplied data.

🎯 Recommendations

High Priority

1. Fix the recursion/guard logic in onPageReady (see Issue TODO for first version #1 above)
2. Add test for interactive state behavior
3. Fix test naming to match actual behavior

Medium Priority

4. Document the performance tradeoff in code comments and potentially in upgrade notes
5. Consider adding a timeout fallback to prevent indefinite waits on broken pages
6. Add integration test that verifies the fix for the original CI failure

Low Priority

7. Consider extracting the magic string 'complete' to a constant
8. Add JSDoc comments explaining why complete is required vs DOMContentLoaded

📊 Overall Assessment

Recommendation: ⚠️ Request Changes

While this PR addresses a real and important bug, the recursive onPageReady call pattern has a critical flaw that needs to be fixed before merging. The test coverage is good but has a naming issue and a missing edge case.

Once the recursion issue is addressed and tests are cleaned up, this will be a solid fix that improves reliability at the cost of slightly slower hydration (which is an acceptable tradeoff for correctness).

🔧 Suggested Fix

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (isPageLifecycleInitialized) {
    return;
  }
  isPageLifecycleInitialized = true;

  if (document.readyState === 'complete') {
    callback();
  } else {
    const handleReadyStateChange = () => {
      if (document.readyState === 'complete') {
        callback();
        document.removeEventListener('readystatechange', handleReadyStateChange);
      }
    };
    document.addEventListener('readystatechange', handleReadyStateChange);
  }
}

This version:

• Avoids recursion
• Explicitly checks for complete state in the handler
• Only removes listener when actually complete
• More clearly expresses the intent

Let me know if you'd like me to clarify any of these points or if you have questions about the suggested changes!

Code Review

Thank you for this PR! I've reviewed the changes to address the component registration race condition. Here's my detailed feedback:

Overview

This PR changes the page lifecycle initialization from waiting for DOMContentLoaded to waiting for readyState === 'complete', ensuring all subresources are loaded before hydration begins. This is a valid approach to prevent race conditions when the Callback Registry was moved to Pro-only.

🔴 Critical Issues

1. Potential Infinite Recursion in onPageReady (pageLifecycle.ts:72-75)

The current implementation has a recursion issue:

document.addEventListener('readystatechange', function onReadyStateChange() {
  onPageReady(callback);  // Recursively calls itself
  document.removeEventListener('readystatechange', onReadyStateChange);
});

Problem: If readystatechange fires multiple times (e.g., loading → interactive → complete), each event could trigger multiple nested onPageReady calls before the listener is removed. The listener is removed AFTER the recursive call, not before.

Recommended fix:

document.addEventListener('readystatechange', function onReadyStateChange() {
  document.removeEventListener('readystatechange', onReadyStateChange);  // Remove first
  onPageReady(callback);  // Then recurse
});

Or better yet, check the state in the listener:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    document.removeEventListener('readystatechange', onReadyStateChange);
    if (!isPageLifecycleInitialized) {
      isPageLifecycleInitialized = true;
      callback();
    }
  }
});

2. Race Condition with Multiple Calls (pageLifecycle.ts:62-77)

The isPageLifecycleInitialized flag is only checked when readyState === 'complete', not when the state is still loading. This means:

• First call to onPageLoaded: readyState is loading → adds listener
• Second call to onPageLoaded: readyState is loading → adds ANOTHER listener
• Both listeners will eventually fire and both will call setupPageNavigationListeners

Impact: Multiple navigation listeners could be registered, causing callbacks to run multiple times.

Recommended fix: Check the flag earlier or use a single shared listener:

let isPageLifecycleInitialized = false;
let isListenerAttached = false;

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;
    }
    isPageLifecycleInitialized = true;
    callback();
  } else if (!isListenerAttached) {
    isListenerAttached = true;
    document.addEventListener('readystatechange', function onReadyStateChange() {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', onReadyStateChange);
        if (!isPageLifecycleInitialized) {
          isPageLifecycleInitialized = true;
          callback();
        }
      }
    });
  }
}

⚠️ High Priority Issues

3. Incomplete Test Coverage (pageLifecycle.test.js)

The removed test at lines 234-254 tested duplicate initialization prevention, which is now even MORE critical given the recursion issue above. This test should be restored and updated, not removed.

Recommendation: Add tests for:

• Multiple calls to onPageLoaded before complete state
• Behavior across loading → interactive → complete transitions
• Ensuring setupPageNavigationListeners is only called once

4. Test Name Mismatch (pageLifecycle.test.js:84)

Line 84: Test is named "should wait for readystatechange when document.readyState is 'interactive'" but sets state to 'loading':

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('loading');  // ← Mismatch!

Fix: Either rename the test or add a separate test for interactive state.

💡 Design Considerations

5. Behavioral Change: DOMContentLoaded vs Complete

This is a significant behavioral change:

• Before: Waited for DOMContentLoaded (DOM ready, but CSS/images/iframes may still be loading)
• After: Waits for readyState === 'complete' (ALL subresources loaded)

Implications:

• ✅ Pro: Prevents race conditions with async component registration
• ⚠️ Con: Hydration happens later, potentially causing more FOUC (Flash of Unstyled Content) or delayed interactivity
• ⚠️ Con: If a page has slow-loading images/iframes, hydration could be significantly delayed

Questions:

1. Have you tested this on pages with slow-loading resources?
2. Is there user-facing documentation that needs updating about when hydration occurs?
3. Could this be made configurable for users who want earlier hydration?

6. Alternative Solution: Load Event

If you want to wait for complete resource loading, the standard load event might be more idiomatic:

if (document.readyState === 'complete') {
  // Already loaded
  callback();
} else {
  window.addEventListener('load', callback, { once: true });
}

This is simpler and avoids the recursion pattern entirely.

📝 Minor Issues

7. TypeScript Type Safety

The onPageReady function accepts callback: () => void but is always called with setupPageNavigationListeners. Consider:

• Renaming to waitForPageReady for clarity
• Adding JSDoc explaining the behavior

8. Duplicate Test (pageLifecycle.test.js:84-108)

Lines 84-95 and 97-108 are nearly identical tests. One should be removed or they should test different states (interactive vs loading).

✅ Positive Aspects

1. Clear problem statement: The PR description clearly explains the race condition issue
2. Focused scope: Changes are minimal and targeted
3. Test updates: Tests were updated to match the new behavior
4. Self-cleaning listener: The listener removes itself after firing

🎯 Recommendations

Immediate (before merge):

1. Fix the recursion issue in onPageReady (Critical TODO for first version #1)
2. Fix the multiple listener issue (Critical Make work with turbolinks and better helper #2)
3. Restore and update the duplicate initialization test (High Add linting and CI scripts #3)
4. Fix the test name mismatch (High Add integration tests #4)

Follow-up (can be separate PR):

1. Consider the load event alternative for cleaner code
2. Add performance monitoring to track hydration delay impact
3. Document the behavioral change for users
4. Consider making this configurable via ReactOnRails.configure

📋 Testing Checklist

Please verify:

• Tests pass locally with yarn run test
• Manual testing on pages with slow-loading images
• Manual testing with Turbo/Turbolinks enabled
• No console errors about missing components
• Hydration timing feels acceptable to users

Would you like me to help implement any of these fixes? I can provide specific code changes for the critical issues.

Reviewed with: Repository CLAUDE.md guidelines
Focus areas: Code quality, race conditions, test coverage, backward compatibility

Code Review for PR #1988

Thank you for addressing the component registration race condition! I've reviewed the changes and have both positive feedback and concerns to share.

✅ Strengths

1. Correct Problem Identification: You've correctly identified that removing the CallbackRegistry in PR PR #4: Split JS Pro Code to Separate Package #1841 broke async hydration support, leading to race conditions where components try to hydrate before webpack bundles finish loading.

2. Sensible Solution: Waiting for document.readyState === 'complete' (all subresources loaded) is the right approach for the open-source version without CallbackRegistry support.

3. Test Updates: Tests are properly updated to reflect the new behavior, changing from DOMContentLoaded to readystatechange event.

🚨 Critical Issues

1. Logic Bug: Infinite Recursion Risk ⚠️

The new onPageReady function has a problematic implementation at pageLifecycle.ts:72-76:

document.addEventListener('readystatechange', function onReadyStateChange() {
  onPageReady(callback);  // Recursively calls itself
  document.removeEventListener('readystatechange', onReadyStateChange);
});

Issue: The function calls onPageReady(callback) recursively, which will re-check readyState. If readyState is still not 'complete', it will add another event listener before removing the current one, potentially leading to multiple listeners being registered.

Recommended Fix:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    document.removeEventListener('readystatechange', onReadyStateChange);
    if (!isPageLifecycleInitialized) {
      isPageLifecycleInitialized = true;
      callback();
    }
  }
});

2. Breaking Change: interactive State Behavior 📋

The old implementation triggered on readyState !== 'loading' (includes 'interactive' and 'complete'), but the new implementation only triggers on readyState === 'complete'.

Behavioral difference:

• Old: Hydration could start when DOM is ready but subresources (images, CSS) still loading ('interactive' state)
• New: Must wait until ALL subresources finish loading ('complete' state)

Impact: This could noticeably delay hydration on pages with large images or slow-loading assets. While this is intentional to fix the race condition, users should be aware of the performance trade-off.

Test discrepancy: The test pageLifecycle.test.js:84-95 was deleted, but the old behavior description says it should run immediately for 'interactive'. The PR changes this but doesn't document it clearly.

3. Lost Feature: Duplicate Initialization Prevention 🔄

The PR removes the test for "preventing duplicate initialization" (pageLifecycle.test.js:236-255), but the actual duplicate prevention logic still exists in the code.

Questions:

• Why was this test removed? Does the new implementation still prevent duplicates correctly?
• The isPageLifecycleInitialized flag is now set inside onPageReady only when readyState === 'complete', but what happens if onPageLoaded is called multiple times while still in 'loading' state? Will multiple readystatechange listeners be added?

🔍 Other Observations

Test Naming Inconsistency

pageLifecycle.test.js:84: Test description says "should wait for readystatechange when document.readyState is 'interactive'" but the test sets readyState to 'loading'. Should this test be renamed or should there be a separate test for 'interactive' state?

Missing Edge Case Test

No test covers what happens when onPageLoaded is called multiple times before readyState === 'complete'. This would help verify duplicate initialization prevention still works.

📊 Performance Considerations

Positive: Using readystatechange with 'complete' check ensures all subresources (JS bundles, images, CSS) are loaded before hydration, eliminating the race condition.

Negative: This may delay time-to-interactive, especially on slow connections. The old behavior allowed hydration to start earlier ('interactive' state), providing faster perceived performance.

Suggestion: Document this trade-off in the CHANGELOG and consider adding a configuration option in the future to let users opt-in to earlier hydration if they handle component registration carefully.

✅ Security & Best Practices

• No security concerns identified
• Event listener properly cleaned up with removeEventListener
• SSR check (typeof window === 'undefined') preserved

📝 Recommendations

1. Fix the recursive logic bug (critical)
2. Add test for multiple onPageLoaded calls before 'complete' state
3. Clarify in PR description: The behavior change from 'interactive' → 'complete' and its performance implications
4. Consider: Keep the duplicate initialization test or add a comment explaining why it was removed
5. Update CHANGELOG: Document this as a behavioral change that may affect hydration timing

Overall Assessment

The PR addresses a real problem and takes the right general approach, but the recursive implementation needs refinement to avoid potential bugs. With the suggested fixes, this will be a solid solution for the open-source version's hydration timing.

Code Review

Overview

This PR changes the page lifecycle initialization from waiting for DOMContentLoaded to waiting for document.readyState === 'complete' (all subresources loaded). This addresses race conditions where React components try to hydrate before their JavaScript bundles are fully loaded.

✅ Strengths

1. Addresses Real Race Condition

The change correctly solves the component registration race condition by ensuring all scripts/resources are loaded before hydration begins. This aligns with the original behavior from PR #1540.

2. Clean Implementation

• Recursive onPageReady function is elegant and handles the state transition correctly
• Proper cleanup with removeEventListener to prevent memory leaks
• Good separation of concerns with the initialization flag

3. Test Updates Are Accurate

The test changes correctly reflect the new behavior:

• Tests now verify readystatechange instead of DOMContentLoaded
• Removed duplicate initialization test (which is no longer applicable)

⚠️ Issues & Concerns

1. Critical Bug: Infinite Recursion Risk 🔴

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;
    }
    isPageLifecycleInitialized = true;
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      document.removeEventListener('readystatechange', onReadyStateChange);
      onPageReady(callback);  // 🔴 Could recurse if readyState goes: loading → interactive → complete
    });
  }
}

Problem: The readystatechange event fires multiple times as document.readyState transitions:

• loading → interactive (DOM parsed)
• interactive → complete (all subresources loaded)

When the event fires for interactive, the function recurses and adds another listener, potentially causing unnecessary recursion or timing issues.

Fix: Add a check for the specific state transition:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;
    }
    isPageLifecycleInitialized = true;
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', onReadyStateChange);
        onPageReady(callback);
      }
    });
  }
}

Alternatively, use the standard load event which fires only once:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;
    }
    isPageLifecycleInitialized = true;
    callback();
  } else {
    window.addEventListener('load', function onLoad() {
      window.removeEventListener('load', onLoad);
      if (isPageLifecycleInitialized) return;
      isPageLifecycleInitialized = true;
      callback();
    });
  }
}

2. Multiple Callback Registration Issue 🟡

Current behavior in pageLifecycle.ts:84 and :92:

export function onPageLoaded(callback: PageLifecycleCallback): void {
  if (currentPageState === 'load') {
    void callback();
  }
  pageLoadedCallbacks.add(callback);
  onPageReady(setupPageNavigationListeners);  // 🟡 Called every time
}

Problem: Every call to onPageLoaded or onPageUnloaded calls onPageReady(setupPageNavigationListeners), which:

• Before initialization: Adds a new event listener each time
• After initialization: Returns early, but still executes the check

Impact: If 10 components call onPageLoaded before the page is ready, you'll have 10 event listeners (though they all remove themselves on first fire).

Fix: Only call onPageReady once:

let hasRequestedPageReady = false;

export function onPageLoaded(callback: PageLifecycleCallback): void {
  if (currentPageState === 'load') {
    void callback();
  }
  pageLoadedCallbacks.add(callback);
  
  if (!hasRequestedPageReady) {
    hasRequestedPageReady = true;
    onPageReady(setupPageNavigationListeners);
  }
}

3. Test Coverage Gap 🟡

The removed test "preventing duplicate initialization" was actually testing important behavior. The new implementation should still prevent issues with multiple registrations, but this is no longer explicitly tested.

Recommendation: Add a test that verifies:

• Multiple onPageLoaded calls before page ready don't cause issues
• setupPageNavigationListeners is only called once
• Event listeners are not duplicated

4. Documentation Missing 📝

Missing context:

• Why complete instead of DOMContentLoaded?
• What race condition does this solve?
• Performance implications (delaying hydration)

Recommendation: Add inline comments explaining the change:

// Wait for document.readyState === 'complete' (not just 'interactive') to ensure
// all JavaScript bundles and subresources are loaded before hydration begins.
// This prevents component registration race conditions where ReactOnRails.register()
// may not have been called yet when we try to hydrate components.
// See: https://github.com/shakacode/react_on_rails/pull/1540
function onPageReady(callback: () => void) {

🔍 Additional Observations

5. Test Description Mismatch

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('loading');  // 🤔 Test name says 'interactive' but sets 'loading'

Fix: Update test name to match:

it('should wait for readystatechange when document.readyState is "loading"', () => {

Or add a separate test for interactive state if that's a meaningful distinction.

6. Performance Consideration ⚡

Waiting for complete delays hydration until all images, stylesheets, fonts, etc. are loaded. For pages with large assets, this could significantly delay interactivity.

Questions:

• Have you tested this on pages with large images or slow-loading assets?
• Is there a way to wait only for JavaScript bundles, not all subresources?
• Could this be made configurable for apps that don't have the race condition?

7. Removed System Tests 🧪

The PR removes 52 lines of system tests for Turbolinks navigation with immediate hydration.

Question: Are these scenarios now covered elsewhere, or are we accepting reduced test coverage for Pro features?

🎯 Recommendations

High Priority (Must Fix)

1. Fix the recursion issue with readystatechange handler
2. Prevent multiple event listener registration in onPageLoaded/onPageUnloaded

Medium Priority (Should Fix)

3. Add test coverage for multiple registration scenario
4. Fix test description mismatch
5. Add inline documentation explaining the race condition

Low Priority (Nice to Have)

6. Consider performance implications and document them
7. Add comment explaining why system tests were removed

💭 Questions for Discussion

1. Have you verified this works correctly when the script is loaded:

   • After interactive but before complete?
   • After complete?

2. What about Turbo Drive / Turbolinks navigation? Does this affect subsequent page navigations where resources might already be cached?

3. Could we use a more targeted approach like waiting for specific script tags rather than all subresources?

Summary

The core approach is sound and addresses a real race condition. However, there are implementation bugs that need fixing before merging:

• ✅ Solves the component registration race condition
• ✅ Clean, understandable code structure
• ❌ Has recursion and multiple listener issues
• ⚠️ May impact performance on slow-loading pages
• ⚠️ Reduced test coverage for Pro features

Verdict: Needs changes before merging. The bugs are fixable with minor adjustments to the event handling logic.

I disagree with Claude & CodeRabbit regarding the recursion. I don't care how many event listeners are created. I only care that they don't actually do anything once isPageLifecycleInitialized is true.

Code Review

Thanks for working on this fix! I've reviewed the changes and here's my feedback:

✅ Strengths

1. Correctly addresses the root cause: Since CallbackRegistry was moved to Pro in PR PR #4: Split JS Pro Code to Separate Package #1841, the open-source version no longer supports async/immediate hydration. Reverting to waiting for full page load is the right approach for preventing race conditions.

2. Improved readiness semantics: Changing from DOMContentLoaded to readystatechange with readyState === 'complete' ensures all subresources (images, stylesheets, scripts) are loaded before hydration, which is more robust.

3. Test coverage updated appropriately: Tests now reflect the new behavior where interactive state waits rather than proceeding immediately.

🔍 Issues & Concerns

Critical: Potential infinite recursion bug

In pageLifecycle.ts:72-75, the readystatechange event handler recursively calls onPageReady():

document.addEventListener('readystatechange', function onReadyStateChange() {
  document.removeEventListener('readystatechange', onReadyStateChange);
  onPageReady(callback);  // ⚠️ Recursive call
});

Problem: If readyState changes from loading → interactive (not complete), this will add another event listener and recurse. While the listener removes itself, this creates unnecessary listener churn and could theoretically stack up calls if the state transitions quickly.

Suggested fix: Check if readyState === 'complete' inside the event handler before recursing:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    document.removeEventListener('readystatechange', onReadyStateChange);
    if (!isPageLifecycleInitialized) {
      isPageLifecycleInitialized = true;
      callback();
    }
  }
});

Behavioral regression: duplicate initialization guard removed

The deleted test preventing duplicate initialization (lines 235-251) was testing an important safeguard. The current implementation at line 66-70 only prevents duplicate initialization when readyState === 'complete', but not when it's loading or interactive.

Issue: If onPageLoaded() is called multiple times before the page is ready, each call will add a new readystatechange listener. While they remove themselves, this is wasteful.

Suggested fix: Move the initialization guard outside the readyState check:

let isPageLifecycleInitialized = false;
function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;
  if (isPageLifecycleInitialized) return;  // Guard at top level
  
  if (document.readyState === 'complete') {
    isPageLifecycleInitialized = true;
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', onReadyStateChange);
        if (!isPageLifecycleInitialized) {
          isPageLifecycleInitialized = true;
          callback();
        }
      }
    });
  }
}

Test coverage gap

The removed test for duplicate initialization should be replaced with a test that verifies the new behavior doesn't add multiple listeners when called repeatedly while readyState is not complete.

📊 Performance Considerations

Positive: Waiting for complete instead of DOMContentLoaded may slightly delay hydration, but it eliminates race conditions. This is the right tradeoff for correctness.

Suggestion: Consider adding a comment explaining why complete is used over DOMContentLoaded to help future maintainers understand the design decision.

🔒 Security Concerns

No security issues identified.

📝 Code Quality

1. Comment clarity: The test comments at lines 91-93 say readyState is 'loading' but the test sets it to 'interactive'. Update for accuracy.

2. Consistency: Good that the same pattern is used for both onPageLoaded and onPageUnloaded.

✅ Recommendations

1. Fix the recursion issue in the readystatechange handler
2. Restore duplicate initialization guard at the function level
3. Add test coverage for multiple calls before page ready
4. Update test comments to match actual state values
5. Add inline comment explaining why complete vs DOMContentLoaded

📋 Style & Conventions

Code follows the project's TypeScript conventions. Remember to run bundle exec rubocop and yarn start format before pushing, as required by CLAUDE.md.

Overall, this PR addresses the right problem with a sensible solution, but needs refinement in the implementation to avoid edge cases and maintain proper guards against duplicate initialization.

Code Review for PR #1988

Thank you for working on this PR to address the component registration race condition. I've reviewed the changes and have the following feedback:

✅ Strengths

1. Clear Problem Statement: The PR correctly identifies that async hydration support (via CallbackRegistry) moved to Pro, creating a race condition for the open-source version.

2. Correct Approach: Waiting for document.readyState === 'complete' instead of DOMContentLoaded ensures all subresources (scripts, stylesheets, images) are loaded before hydration, which prevents component registration race conditions.

3. Test Updates Match Implementation: The test changes correctly reflect the new behavior - waiting for readystatechange instead of DOMContentLoaded.

🔍 Issues & Concerns

1. Critical Logic Error in onPageReady Function

The new implementation has a significant flaw in the recursive event listener pattern:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;  // ❌ PROBLEM: Returns without calling callback
    }
    isPageLifecycleInitialized = true;
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      document.removeEventListener('readystatechange', onReadyStateChange);
      onPageReady(callback);  // Recursive call
    });
  }
}

Problem: When onPageReady is called multiple times (e.g., from both onPageLoaded and onPageUnloaded), subsequent calls after initialization will return early without executing the callback. This breaks the initialization flow.

Expected Behavior: The old code initialized listeners once but allowed callbacks to be registered and executed. The new code prevents proper initialization on subsequent calls.

Suggested Fix: The isPageLifecycleInitialized check should be moved outside the function or handled differently:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;
  
  if (isPageLifecycleInitialized) {
    callback();  // If already initialized, just run callback
    return;
  }

  if (document.readyState === 'complete') {
    isPageLifecycleInitialized = true;
    callback();
  } else {
    document.addEventListener('readystatechange', function onReadyStateChange() {
      document.removeEventListener('readystatechange', onReadyStateChange);
      onPageReady(callback);
    });
  }
}

2. Behavior Change for interactive State

Old Behavior:

• readyState !== 'loading' includes both 'interactive' and 'complete'
• DOMContentLoaded fires during 'interactive' state (DOM ready, subresources may still be loading)
• This allowed hydration to start as soon as DOM was parsed

New Behavior:

• Only readyState === 'complete' triggers immediate initialization
• 'interactive' state now waits for readystatechange event
• This delays hydration until ALL subresources are loaded

Impact: This is more conservative and safer for race conditions, but may slightly delay time-to-interactive. This tradeoff seems acceptable given the race condition issue, but should be documented.

3. Test Coverage Gap

The removed test (preventing duplicate initialization) was actually testing important behavior. While the old test may not match the new implementation, you should add a new test that verifies:

• Multiple calls to onPageLoaded / onPageUnloaded don't cause multiple readystatechange listeners
• The initialization flag properly prevents redundant setup

Suggested test:

it('should handle multiple onPageReady calls without duplicate initialization', () => {
  setReadyState('complete');
  const { onPageLoaded, onPageUnloaded } = importPageLifecycle();
  
  onPageLoaded(jest.fn());
  onPageUnloaded(jest.fn());
  
  // Should only initialize once
  expect(addEventListenerSpy).not.toHaveBeenCalledWith('readystatechange', expect.any(Function));
});

4. Removed Integration Tests

The two removed integration tests (Turbolinks across pages and TurboStream send react component) were testing Pro-specific immediate_hydration functionality. However:

Question: Are these tests moved to the Pro package, or are they no longer needed? The PR description doesn't clarify this.

Recommendation:

• If these tests are important for Pro functionality, they should be added to the Pro package test suite
• If immediate hydration is no longer supported even in Pro, this should be documented in the CHANGELOG

5. Missing Event Listener Cleanup

The recursive readystatechange pattern is correctly cleaning up listeners with removeEventListener, but consider edge cases:

• What happens if the document state changes rapidly?
• Are there any scenarios where the listener might not be cleaned up?

The current implementation looks correct, but adding a comment explaining the recursive pattern would help future maintainers.

🎯 Performance Considerations

Positive: Waiting for complete state prevents race conditions and ensures consistent behavior.

Potential Concern: Slightly longer time-to-interactive compared to DOMContentLoaded, especially on pages with large images or slow-loading assets.

Recommendation: Consider adding a configuration option to allow advanced users to opt back into DOMContentLoaded behavior if needed (future enhancement).

🔒 Security

No security concerns identified. The changes are limited to page lifecycle management and don't introduce new attack vectors.

📋 Recommendations

Must Fix:

1. Fix the isPageLifecycleInitialized logic bug in onPageReady function
2. Add test coverage for duplicate initialization prevention

Should Address:
3. Clarify what happened to the Pro integration tests (moved vs. deleted)
4. Add code comments explaining the recursive readystatechange pattern
5. Update CHANGELOG.md with this user-visible behavior change (components now hydrate after all subresources load)

Nice to Have:
6. Consider documenting the performance tradeoff in code comments
7. Add a test that simulates the race condition this fixes

📝 Style & Convention Notes

Per CLAUDE.md:

• ✅ Uses TypeScript with proper type annotations
• ✅ Test descriptions updated to match new behavior
• ⚠️ CHANGELOG.md should be updated (use /update-changelog command)
• ⚠️ Ensure bundle exec rubocop passes before pushing

Let me know if you'd like help with any of these fixes!

Code Review: Only initiate hydration after all subresources loaded

Overview

This PR reverts the hydration behavior to wait for document.readyState === 'complete' instead of 'interactive' or DOMContentLoaded, addressing race conditions where components may attempt to hydrate before their JavaScript bundles are fully loaded.

Code Quality: ✅ Good

Strengths:

1. Clear intent: The change directly addresses the stated problem of component registration race conditions
2. Clean implementation: The refactored onPageReady() function is more focused and easier to understand
3. Proper cleanup: The readystatechange listener is correctly removed after firing

Areas for Improvement:

1. Potential Infinite Recursion Risk ⚠️

packages/react-on-rails/src/pageLifecycle.ts:72-74

The recursive call to onPageReady(callback) inside the readystatechange listener could theoretically cause issues:

document.addEventListener('readystatechange', function onReadyStateChange() {
  document.removeEventListener('readystatechange', onReadyStateChange);
  onPageReady(callback);  // Recursive call
});

Issue: If readyState changes from 'loading' → 'interactive' (not 'complete'), the function will recursively add another listener. While this will eventually resolve when 'complete' is reached, it's not ideal.

Recommendation: Use a loop or check explicitly for 'complete':

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;

  if (document.readyState === 'complete') {
    if (isPageLifecycleInitialized) {
      return;
    }
    isPageLifecycleInitialized = true;
    callback();
  } else {
    const checkReady = () => {
      if (document.readyState === 'complete') {
        document.removeEventListener('readystatechange', checkReady);
        onPageReady(callback);
      }
    };
    document.addEventListener('readystatechange', checkReady);
  }
}

2. Inconsistent Guard Logic

packages/react-on-rails/src/pageLifecycle.ts:67-69

The isPageLifecycleInitialized guard is only checked when readyState === 'complete'. If multiple calls happen while readyState !== 'complete', multiple listeners could be added.

Recommendation: Move the initialization check to the beginning:

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;
  
  if (isPageLifecycleInitialized) {
    return;
  }

  if (document.readyState === 'complete') {
    isPageLifecycleInitialized = true;
    callback();
  } else {
    // ... listener code
  }
}

Test Coverage: ⚠️ Concerns

Issue: Tests No Longer Match Reality

The test changes reveal a semantic mismatch:

Old behavior (DOMContentLoaded):

• 'loading' → wait for DOMContentLoaded
• 'interactive' → already past DOMContentLoaded, proceed immediately
• 'complete' → proceed immediately

New behavior (complete only):

• 'loading' → wait for 'complete'
• 'interactive' → wait for 'complete' ← This is the key change
• 'complete' → proceed immediately

However, the test for 'interactive' now expects to wait:

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('interactive');
  const callback = jest.fn();
  const { onPageLoaded } = importPageLifecycle();

  onPageLoaded(callback);

  expect(callback).not.toHaveBeenCalled();
  expect(addEventListenerSpy).toHaveBeenCalledWith('readystatechange', expect.any(Function));
});

Problem: This test will never verify that the callback eventually fires because:

1. The test sets readyState to 'interactive'
2. Adds a readystatechange listener
3. But never simulates the state changing to 'complete'
4. The callback remains pending indefinitely

Recommendation: Add a follow-up assertion that simulates the state change:

it('should wait for readystatechange when document.readyState is "interactive"', () => {
  setReadyState('interactive');
  const callback = jest.fn();
  const { onPageLoaded } = importPageLifecycle();

  onPageLoaded(callback);

  expect(callback).not.toHaveBeenCalled();
  
  // Capture the listener that was added
  const readystateListener = addEventListenerSpy.mock.calls
    .find(call => call[0] === 'readystatechange')[1];
  
  // Simulate state change to complete
  setReadyState('complete');
  readystateListener();
  
  // Now callback should have fired
  expect(callback).toHaveBeenCalledTimes(1);
});

Removed Test Coverage

The removal of the "preventing duplicate initialization" test is concerning. While it may not have been testing the exact new behavior, the concept of preventing duplicate initialization is still important.

Recommendation: Add a new test that verifies multiple onPageLoaded calls while in non-complete state don't cause issues.

Integration Tests: 🔴 Major Concern

Removed Tests Without Replacement

spec/dummy/spec/system/integration_spec.rb:89-138

Two entire test scenarios were removed:

1. Turbolinks navigation test - "changes name in message according to input"
2. TurboStream component test - "force load hello-world component immediately"

Problem: These tests were validating actual user-facing behavior with Pro features. Their removal means:

• Loss of regression protection for immediate hydration in Turbo/Turbolinks contexts
• No validation that the new behavior doesn't break Pro features
• Unclear if these scenarios still work at all

Questions:

1. Do these features still work with the new implementation?
2. Were these tests only removed because they depend on immediate_hydration config?
3. Should they be moved to the Pro package's test suite instead?

Recommendation:

• If Pro package has its own integration tests that cover this, add a comment referencing them
• If not, these tests should be adapted to work with the new behavior or moved to Pro
• At minimum, document why it's safe to remove these tests

Performance Considerations: ⚠️ Trade-off

Change Impact

• Before: Hydration could start at DOMContentLoaded (when DOM is ready)
• After: Hydration must wait for load event (all resources loaded)

Pros:
✅ Eliminates race conditions with component registration
✅ Ensures all JavaScript bundles are loaded before hydration

Cons:
⚠️ Slower perceived performance: Users may see a longer delay before interactive content
⚠️ Blocking on unrelated resources: Hydration now waits for ALL resources (images, fonts, etc.), not just scripts

Real-world impact example:

• Page has large images or slow third-party resources
• DOM and scripts load quickly
• Under old behavior: Components hydrate quickly (good UX)
• Under new behavior: Must wait for images/fonts (degraded UX)

Recommendation: Consider alternative approaches:

1. Use a module/script load tracking mechanism instead of readyState
2. Implement a timeout to prevent waiting too long
3. Document this trade-off in the changelog as a breaking change affecting performance

Security: ✅ No Concerns

No security implications identified. The changes are purely about timing of initialization.

Best Practices Alignment: ⚠️ Partially

Adherence to CLAUDE.md:

• ✅ Code changes follow TypeScript patterns
• ✅ Test structure maintained
• ⚠️ Missing: No CHANGELOG.md entry (should be added per CLAUDE.md)
• ⚠️ Missing: No update to documentation about behavior change

Recommendations Summary

Critical (Must Address):

1. Fix infinite recursion potential in onPageReady()
2. Complete the test for 'interactive' state to verify callback eventually fires
3. Explain or restore integration tests - document why they were removed

Important (Should Address):

4. Add CHANGELOG.md entry - this is a user-visible behavior change
5. Consider performance implications - document the trade-off or explore alternatives
6. Add test for duplicate initialization prevention

Nice to Have:

7. Consider adding a configuration option to allow users to choose between 'interactive' and 'complete'
8. Add JSDoc comments explaining the timing guarantees

Final Verdict: ⚠️ Needs Work Before Merge

While the core fix is sound and addresses a real problem, the implementation has edge cases that should be addressed, and the removal of integration tests without explanation is concerning. The performance trade-off should also be carefully considered and documented.

Suggested next steps:

1. Address the recursion and guard logic issues
2. Complete test coverage for the 'interactive' state path
3. Clarify the integration test removal
4. Add changelog entry
5. Consider adding performance metrics/testing

Great work identifying and addressing the race condition! With these refinements, this will be a solid fix. 🚀

Code Review - PR #1988: Only initiate hydration after all subresources loaded

Overview

This PR addresses a critical race condition by ensuring React component hydration only occurs after the document is fully loaded (readyState === 'complete'), rather than just after DOM is ready (DOMContentLoaded). This prevents component registration race conditions that were occurring in CI.

✅ Strengths

1. Correct Problem Identification: The change correctly identifies that without the CallbackRegistry (moved to Pro in PR #4: Split JS Pro Code to Separate Package #1841), the core package needs synchronous hydration after all resources load.

2. Proper Event Usage: Switching from DOMContentLoaded to readyState === 'complete' ensures all subresources (scripts, stylesheets, images) are loaded before hydration begins.

3. Clean Test Updates: Tests appropriately updated to reflect the new semantics - callbacks wait for complete document readiness.

4. Removes Invalid Tests: Correctly removes tests for immediate hydration features that are now Pro-only.

⚠️ Critical Issues

1. Logic Bug: Recursive Event Listener Pattern

Location: pageLifecycle.ts:62-68

document.addEventListener('readystatechange', function onReadyStateChange() {
  document.removeEventListener('readystatechange', onReadyStateChange);
  onPageReady(callback);  // ⚠️ Recursively calls itself
});

Problem: This creates a recursive pattern that will re-check document.readyState on EVERY state change (loading → interactive → complete). While it eventually works, it's inefficient and confusing.

Better approach:

document.addEventListener('readystatechange', function onReadyStateChange() {
  if (document.readyState === 'complete') {
    document.removeEventListener('readystatechange', onReadyStateChange);
    if (!isPageLifecycleInitialized) {
      isPageLifecycleInitialized = true;
      callback();
    }
  }
});

Or use the standard window.addEventListener('load', callback, { once: true }) pattern which fires when readyState === 'complete'.

2. Unguarded Global State Mutation

Location: pageLifecycle.ts:66-68

The isPageLifecycleInitialized flag is only set/checked inside the readyState === 'complete' branch. If onPageReady is called multiple times while the document is still loading, multiple event listeners will be registered.

Test case missing: What happens if:

onPageLoaded(callback1); // readyState = 'loading'
onPageLoaded(callback2); // readyState = 'loading' (still)
onPageLoaded(callback3); // readyState = 'loading' (still)

This will register 3 separate readystatechange listeners, each of which will recursively call onPageReady again.

🔍 Code Quality Concerns

3. Inconsistent Naming

The function is named onPageReady but it's actually checking/waiting for document.readyState === 'complete', which is the load event, not just "page ready". Consider:

• onDocumentComplete
• onAllResourcesLoaded
• waitForCompleteLoad

4. Missing Documentation

The change fundamentally alters when hydration occurs (DOMContentLoaded → load event). This is a breaking behavioral change that could affect timing-sensitive code. Should document:

• Why the change was necessary
• What the timing difference means for users
• Potential performance implications (components render later)

🧪 Test Coverage Issues

5. Removed Critical Test

The test "preventing duplicate initialization" was removed, but this functionality is still important. The new code should still prevent duplicate listener registration.

Recommend: Add a test like:

it('should not register multiple readystatechange listeners', () => {
  setReadyState('loading');
  const { onPageLoaded } = importPageLifecycle();
  
  onPageLoaded(jest.fn());
  onPageLoaded(jest.fn());
  onPageLoaded(jest.fn());
  
  // Should only register one listener
  const readyStateListeners = addEventListenerSpy.mock.calls
    .filter(([event]) => event === 'readystatechange');
  expect(readyStateListeners.length).toBe(1);
});

6. No Test for Multiple State Transitions

There's no test verifying the recursive behavior works correctly through multiple readystatechange events.

🚀 Performance Considerations

1. Delayed Hydration: Components now hydrate later (at load instead of DOMContentLoaded). On pages with many/large images, this could delay interactivity significantly.

2. Multiple Recursive Calls: The current implementation may call onPageReady twice (once at 'interactive', once at 'complete'), adding unnecessary overhead.

🔒 Security Concerns

No security issues identified.

📋 Recommendations

Must Fix:

1. Fix the recursive event listener pattern to check readyState within the listener
2. Add guard to prevent multiple listener registrations
3. Add test for duplicate registration prevention

Should Fix:

1. Add JSDoc comments explaining the timing change
2. Consider using standard window.addEventListener('load') for clarity
3. Add test for multiple readystatechange transitions
4. Update CHANGELOG.md if this affects user behavior

Nice to Have:

1. Rename function to better reflect "complete" vs "ready" semantics
2. Add performance monitoring to measure hydration delay impact

💡 Alternative Implementation

Consider this cleaner approach:

let isPageLifecycleInitialized = false;

function onPageReady(callback: () => void) {
  if (typeof window === 'undefined') return;
  
  if (isPageLifecycleInitialized) return;
  
  if (document.readyState === 'complete') {
    isPageLifecycleInitialized = true;
    callback();
  } else {
    // Standard pattern: 'load' event fires when readyState becomes 'complete'
    window.addEventListener('load', () => {
      if (!isPageLifecycleInitialized) {
        isPageLifecycleInitialized = true;
        callback();
      }
    }, { once: true });
  }
}